Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

StringSegment behavior #55501

Merged
merged 6 commits into from
Jul 15, 2021
Merged

StringSegment behavior #55501

merged 6 commits into from
Jul 15, 2021

Conversation

hrrrrustic
Copy link
Contributor

fix #45021
fix #53727

@ghost
Copy link

ghost commented Jul 12, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

fix #45021
fix #53727

Author: hrrrrustic
Assignees: -
Labels:

area-Extensions-Primitives
Milestone: -

@hrrrrustic hrrrrustic changed the title String segment behavior StringSegment behavior Jul 12, 2021
@hrrrrustic @gfoidl
Apply suggestions from code review
b96959f 
Co-authored-by: Günther Foidl <gue@korporal.at>
@GrabYourPitchforks
Copy link
Member

@maryamariyan You marked #45021 as up-for-grabs but never specified what you believe the correct behavior should be: consistently return -1 or consistently throw an exception.

/cc @halter73

This was referenced Jul 13, 2021
Closed
Closed
@hrrrrustic
Copy link
Contributor Author

I guess -1 is the correct way as it described in all of this methods docs 😄

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 447 in 57bfe47

/// <returns>The zero-based index position of <paramref name="c"/> from the beginning of the <see cref="StringSegment"/> if that character is found, or -1 if it is not.</returns>

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 504 in 57bfe47

/// was found; -1 if no character in <paramref name="anyOf"/> was found.</returns>

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 571 in 57bfe47

/// <returns>The zero-based index position of value if that character is found, or -1 if it is not.</returns>

eerhardt
eerhardt approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. @GrabYourPitchforks - any thoughts/concerns?

@eerhardt
Copy link
Member

Linq.Expression failures are #55536. Merging.

@eerhardt eerhardt merged commit 188c270 into dotnet:main Jul 15, 2021
@eerhardt
Copy link
Member

Thanks @hrrrrustic for the contribution here!

@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gfoidl gfoidl gfoidl left review comments

@eerhardt eerhardt eerhardt approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

Assignees
No one assigned
Labels
area-Extensions-Primitives
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StringSegment IndexOf behavior StringSegment doesn't handle null buffers consistently
5 participants
@hrrrrustic @GrabYourPitchforks @eerhardt @gfoidl @maryamariyan